Fix: Fix valset pref unbonding bugs #5967

ValarDragon · 2023-08-06T01:07:11Z

Closes: #6028

This PR is aiming to fix the following two bugs in valset-preference unbonding:

Sometimes selecting more to unbond than the user has
Correctly unbonding if the user's delegations aren't according to valset

The solution to both is the same: Get the delegator's existing delegation to each val in valset, which does increase gas.

ValarDragon · 2023-08-07T02:10:05Z

@stackman27 taking over this PR (or should I get this mergeable (just fixing conflicts), and then you implement the remainder in a followup?

stackman27 · 2023-08-07T17:05:41Z

@stackman27 taking over this PR (or should I get this mergeable (just fixing conflicts), and then you implement the remainder in a followup?

its okay i can just work on this PR and have 1 PR that fixes

stackman27 · 2023-08-08T02:49:32Z

x/valset-pref/validator_set.go

-		totalAmountFromWeights = totalAmountFromWeights.Add(val.Weight.Mul(tokenAmt))
-	}
+	// Step 7
+	fmt.Println("Undelegating the remaining amount.")


note: i'm a little worried about this part of the algorithm. would appreciate a thorough review here

Which part in specific are you worried about?

stackman27 · 2023-08-08T02:52:24Z

x/valset-pref/validator_set.go

+	targetRatio := sdk.OneDec()
+	amountRemaining := coin.Amount
+
+	fmt.Println("VRATIO PRE: ", valSetRatio)


note: these Println are easier to debug/review. Will remove them once this PR is close to merge

ValarDragon · 2023-08-08T03:35:28Z

x/valset-pref/README.md

+Algorithm for undelegation; 
+// unbond as true to valset ratios as possible. Undelegation should be possible.


We should add my intuition description for the algorithm back!

re-added: 90e9fb2#diff-a6daa0f91df9925cade0547638fc212684689d2b2b8c8f9fa89c1c5a7e505e96R47

Is there a reason all of this is commented?

@czarcas7ic this was just the pseudocode. I wrote the algorithm here: https://github.com/osmosis-labs/osmosis/pull/5967/files#diff-4422f5e761e99a0e29d2ba7017ece4aa6b486b5ee24b18fb58fcbc9c26a8e7a6R133

following the steps

x/valset-pref/validator_set.go

ValarDragon · 2023-08-08T03:37:09Z

x/valset-pref/validator_set.go

+	// Step 1-2
+	var valSetRatio []ValRatio


Why did we remove the helper I had before? It improves gas costs by removing repeated Get's with GetDelegationPreferences, and makes this main function cleaner?

i did this for couple of reason

the helper func was useful except i wanted current delegations to be represented as valset {valAddr, weight} and the helper func did the opposite. (represented valset as current delegation)

I checked the flow and current func i'm using has less Get calls to staking keeper than the helper function.

(helper = getValset -> if exist -> getDelegation -> else -> GetDelegatorDelegation, nonHelper = getValSet -> if exist return -> else -> GetDelegatorDelegations)

Ah interesting, didn't notice the delegations call being reused my bad

Err, the delegations aren't getting reused rightnow if you don't have a validator set. This is causing more gas usage.

if you dont have a validator set but have existing delegations we create a validator set for you with existing delegations
ref: https://github.com/osmosis-labs/osmosis/pull/5967/files#diff-eca64719739dd13c273495f6d28e8da96083fdf0c629da0eef1a22206839ed24R84

note: we create temporary valset but donot store it in state, to store it in state you just call SetValSetPref. If valSet exist and delegation also exist and you want to store temporary valset in state this implies ValSetRedelegation

stackman27 · 2023-08-08T17:16:29Z

devbot add changelog fix ValSet undelegate API out of sync with existing staking

stackman27 · 2023-08-08T19:38:16Z

@ValarDragon i'm a little worried that we might have this issue in redelegation too, since it requires tokens to be unbonded. I haven't dug deep into it but if you think dont think this is the case i can focus on other work

x/valset-pref/README.md

x/valset-pref/validator_set.go

mattverse · 2023-10-01T07:10:59Z

Status update: Working on a PR for improvising test cases and refactoring, will get is in as Korean Holidays are over!

mattverse · 2023-10-04T11:03:50Z

#6630

* Add Test cases and fix * Update x/valset-pref/keeper.go * Adams comment * add error types and tidy test --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]>

czarcas7ic · 2023-10-05T18:16:28Z

I looked over this again, and everything except for not being able to reach steps 5-7 makes sense to me. Once we get an answer to this as well as tests if it is in fact reachable, I can approve

Steps 5-7 appear unreachable

* initial push * add todo * msg server test * add godoc * custom error * update godoc * update proto and remove duplication * add more TODOs * use min when calculating amt to withdraw from last val

x/valset-pref/validator_set.go

ValarDragon · 2023-10-13T19:11:22Z

x/valset-pref/keeper.go

+	for _, existingDelegation := range delegations {
+		totalShares = totalShares.Add(existingDelegation.Shares)


Wait... is this the code for valset preference? I think this is fundamentally wrong. This needs to be based on the token amount, not the number of shares

ValarDragon · 2023-10-13T19:22:11Z

I think the last bug should be fixed, but in a separate PR. Otw this PR looks good to go to me

AlpinYukseloglu

Agreed with merging this and fixing the share-based issue in a separate PR

* Add helper to get existing delegations along with valset pref * Lay out more of the logic nuance * Add pseudo-code needed * algorithm v1 * added algorithm docs * fixed all test * removed unwanted files * remove more code * added more tests * update changelog * added test and addressed feedback * Update x/valset-pref/validator_set.go * Update x/valset-pref/validator_set.go * Update x/valset-pref/README.md * Minor cleanup * re-use validator gets * Refactor * Highlight bug * fixed the issue * added comments * update changelog * fixed the issue * fixed go test * fixed test * lint * Finish ValSet Pref Unbonding (#6630) * Add Test cases and fix * Update x/valset-pref/keeper.go * Adams comment * add error types and tidy test --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> * feat: unbond with rebalanced val set weights (#6685) * initial push * add todo * msg server test * add godoc * custom error * update godoc * update proto and remove duplication * add more TODOs * use min when calculating amt to withdraw from last val * Update x/valset-pref/validator_set.go --------- Co-authored-by: stackman27 <[email protected]> Co-authored-by: devbot-wizard <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: roman <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Adam Tucker <[email protected]> (cherry picked from commit e12011b)

* Add helper to get existing delegations along with valset pref * Lay out more of the logic nuance * Add pseudo-code needed * algorithm v1 * added algorithm docs * fixed all test * removed unwanted files * remove more code * added more tests * update changelog * added test and addressed feedback * Update x/valset-pref/validator_set.go * Update x/valset-pref/validator_set.go * Update x/valset-pref/README.md * Minor cleanup * re-use validator gets * Refactor * Highlight bug * fixed the issue * added comments * update changelog * fixed the issue * fixed go test * fixed test * lint * Finish ValSet Pref Unbonding (#6630) * Add Test cases and fix * Update x/valset-pref/keeper.go * Adams comment * add error types and tidy test --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> * feat: unbond with rebalanced val set weights (#6685) * initial push * add todo * msg server test * add godoc * custom error * update godoc * update proto and remove duplication * add more TODOs * use min when calculating amt to withdraw from last val * Update x/valset-pref/validator_set.go --------- Co-authored-by: stackman27 <[email protected]> Co-authored-by: devbot-wizard <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: roman <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Adam Tucker <[email protected]> (cherry picked from commit e12011b) Co-authored-by: Dev Ojha <[email protected]>

* Add helper to get existing delegations along with valset pref * Lay out more of the logic nuance * Add pseudo-code needed * algorithm v1 * added algorithm docs * fixed all test * removed unwanted files * remove more code * added more tests * update changelog * added test and addressed feedback * Update x/valset-pref/validator_set.go * Update x/valset-pref/validator_set.go * Update x/valset-pref/README.md * Minor cleanup * re-use validator gets * Refactor * Highlight bug * fixed the issue * added comments * update changelog * fixed the issue * fixed go test * fixed test * lint * Finish ValSet Pref Unbonding (#6630) * Add Test cases and fix * Update x/valset-pref/keeper.go * Adams comment * add error types and tidy test --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]> * feat: unbond with rebalanced val set weights (#6685) * initial push * add todo * msg server test * add godoc * custom error * update godoc * update proto and remove duplication * add more TODOs * use min when calculating amt to withdraw from last val * Update x/valset-pref/validator_set.go --------- Co-authored-by: stackman27 <[email protected]> Co-authored-by: devbot-wizard <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: roman <[email protected]> Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Adam Tucker <[email protected]>

Add helper to get existing delegations along with valset pref

fe7290f

github-actions bot added the C:docs Improvements or additions to documentation label Aug 6, 2023

ValarDragon added 2 commits August 6, 2023 16:47

Lay out more of the logic nuance

fcdc961

Add pseudo-code needed

58792cc

stackman27 self-assigned this Aug 7, 2023

stackman27 added 6 commits August 7, 2023 21:37

algorithm v1

2f8dea8

added algorithm docs

f4c0dab

fixed all test

144f0d1

removed unwanted files

41cd2f6

remove more code

7403e06

added more tests

fbd4ba4

stackman27 reviewed Aug 8, 2023

View reviewed changes

stackman27 added V:state/breaking State machine breaking PR A:no-changelog labels Aug 8, 2023

stackman27 marked this pull request as ready for review August 8, 2023 02:53

stackman27 requested a review from mattverse as a code owner August 8, 2023 02:53

stackman27 changed the title ~~WIP: Fix valset pref unbonding bugs~~ Fix: Fix valset pref unbonding bugs Aug 8, 2023

ValarDragon commented Aug 8, 2023

View reviewed changes

x/valset-pref/validator_set.go Outdated Show resolved Hide resolved

ValarDragon commented Aug 8, 2023

View reviewed changes

update changelog

2f5caca

devbot-wizard removed the A:no-changelog label Aug 8, 2023

stackman27 added 2 commits August 8, 2023 10:22

Merge branch 'main' into dev/valset-pref-unbond-issues

d3cec49

added test and addressed feedback

90e9fb2

czarcas7ic reviewed Aug 9, 2023

View reviewed changes

x/valset-pref/README.md Outdated Show resolved Hide resolved

x/valset-pref/validator_set.go Outdated Show resolved Hide resolved

x/valset-pref/validator_set.go Outdated Show resolved Hide resolved

mattverse unassigned stackman27 Sep 21, 2023

p0mvn assigned czarcas7ic and p0mvn Sep 26, 2023

p0mvn added 2 commits September 27, 2023 19:45

Merge branch 'main' into dev/valset-pref-unbond-issues

365b721

lint

31e9008

czarcas7ic removed their assignment Sep 28, 2023

Merge branch 'main' into dev/valset-pref-unbond-issues

1c48875

mattverse and others added 2 commits October 5, 2023 12:20

Finish ValSet Pref Unbonding (#6630)

9e98292

* Add Test cases and fix * Update x/valset-pref/keeper.go * Adams comment * add error types and tidy test --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]>

Merge branch 'main' into dev/valset-pref-unbond-issues

eedd243

p0mvn mentioned this pull request Oct 11, 2023

refactor/test(valsetpref): fix issue with not being able to get to steps 5-7 #6664

Closed

czarcas7ic added the A:backport/v20.x backport patches to v20.x branch label Oct 12, 2023

feat: unbond with rebalanced val set weights (#6685)

ce280e8

* initial push * add todo * msg server test * add godoc * custom error * update godoc * update proto and remove duplication * add more TODOs * use min when calculating amt to withdraw from last val

github-actions bot added the C:CLI label Oct 13, 2023

Merge branch 'main' into dev/valset-pref-unbond-issues

30af740

ValarDragon commented Oct 13, 2023

View reviewed changes

x/valset-pref/validator_set.go Outdated Show resolved Hide resolved

Update x/valset-pref/validator_set.go

246d95e

ValarDragon commented Oct 13, 2023

View reviewed changes

AlpinYukseloglu approved these changes Oct 13, 2023

View reviewed changes

AlpinYukseloglu merged commit e12011b into main Oct 13, 2023
1 check passed

AlpinYukseloglu deleted the dev/valset-pref-unbond-issues branch October 13, 2023 19:35

mergify bot mentioned this pull request Oct 13, 2023

Fix: Fix valset pref unbonding bugs (backport #5967) #6696

Merged

github-actions bot mentioned this pull request Oct 15, 2024

Broken Links Detected notional-labs/osmosis#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Fix valset pref unbonding bugs #5967

Fix: Fix valset pref unbonding bugs #5967

ValarDragon commented Aug 6, 2023 •

edited by czarcas7ic

Loading

ValarDragon commented Aug 7, 2023

stackman27 commented Aug 7, 2023

stackman27 Aug 8, 2023

mattverse Aug 23, 2023

stackman27 Aug 8, 2023

ValarDragon Aug 8, 2023

stackman27 Aug 8, 2023

czarcas7ic Aug 9, 2023

stackman27 Aug 11, 2023

ValarDragon Aug 8, 2023

stackman27 Aug 8, 2023 •

edited

Loading

ValarDragon Aug 10, 2023

ValarDragon Aug 13, 2023

stackman27 Aug 15, 2023 •

edited

Loading

stackman27 commented Aug 8, 2023

stackman27 commented Aug 8, 2023 •

edited

Loading

mattverse commented Oct 1, 2023

mattverse commented Oct 4, 2023

czarcas7ic commented Oct 5, 2023

ValarDragon Oct 13, 2023

ValarDragon commented Oct 13, 2023

AlpinYukseloglu left a comment

		Algorithm for undelegation;
		// unbond as true to valset ratios as possible. Undelegation should be possible.

		for _, existingDelegation := range delegations {
		totalShares = totalShares.Add(existingDelegation.Shares)

Fix: Fix valset pref unbonding bugs #5967

Fix: Fix valset pref unbonding bugs #5967

Conversation

ValarDragon commented Aug 6, 2023 • edited by czarcas7ic Loading

ValarDragon commented Aug 7, 2023

stackman27 commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

stackman27 commented Aug 8, 2023

stackman27 commented Aug 8, 2023 • edited Loading

mattverse commented Oct 1, 2023

mattverse commented Oct 4, 2023

czarcas7ic commented Oct 5, 2023

Choose a reason for hiding this comment

ValarDragon commented Oct 13, 2023

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

ValarDragon commented Aug 6, 2023 •

edited by czarcas7ic

Loading

stackman27 Aug 8, 2023 •

edited

Loading

stackman27 Aug 15, 2023 •

edited

Loading

stackman27 commented Aug 8, 2023 •

edited

Loading